add yarg’s strict() to chain. add walletUrl, contractName as options #265

mikedotexe · 2020-02-06T19:20:33Z

Different approach to #259
Also enabled recommendations. So near fake will fail, list possible commands, and say, "Did you mean near stake?"
I think the issue was .config(config) loaded src/config.js which has keys that weren't explicitly set as options.

~~This is quite separate from the PR where most of the discussion on this issue has been happening.~~
#262

If we choose to merge this PR, we can close the other.

Update: this PR has been languishing and Illia brought up that this is important. I see that @vgrichina suggests we overhaul how config is used. Can we get something done in the meantime?

Tests have been failing and I have commits fixing these, including some outdated shell commands. Kind of mysterious that tests have worked at all. I guess some errant calls technically don't exit(1)? Doesn't matter too much, this is ready for re-review.

bin/near-cli.js

vgrichina · 2020-02-06T22:18:28Z

I think the issue was .config(config) loaded src/config.js which has keys that weren't explicitly set as options.

yes, that was the problem

not sure if it's not confusing to keep these as options for all commands (as most commands would ignore --contractName for example) or also what may happen given that it's alias to --accountId – we might end up using value from src/config.js for commands which don't operate on contracts.

for this reason I feel that long-term we need to figure out solution where we load config in different way

vgrichina · 2020-02-06T22:19:58Z

@mikedotexe this is failing the tests BTW

mikedotexe · 2020-02-06T22:30:45Z

@mikedotexe this is failing the tests BTW

Yes, I haven't seen any PRs pass tests the past two days.

Unknown argument: masterAccount
++RESULT='{ networkId: '\''shared-test'\'',
  nodeUrl: '\''http://shared-test.nearprotocol.com:3030'\'',
  contractName: undefined,
  masterAccount: '\''test.near'\'' }'
./test/test_generate_key.sh FAIL
error Command failed with exit code 1.

masterAccount is not something I've touched, which is odd.
Locally, when I run rm -rf neardev && yarn test on master it's also failing, but for a different reason.

Account has existing key pair with ed25519:EsyKEPeLbgPk9qorxLZ2thLghLXJXdfMubF8muy5mg2j public key =~ .*Account has existing key pair with ed25519:.+ public key.* ]]
./test/test_generate_key.sh SUCCESS
error Command failed with exit code 1.

I've had to somewhat ignore errors as it seems something is awry.

mikedotexe · 2020-02-06T22:50:39Z

The latest commit reduces that confusion. When a user types near or near --help it does not show the walletUrl or contractName flags.
I hear what you're saying, @vgrichina, about loading the config. I think this approach will work. I also sense there are going to be a significant changes to shell that are coming soonish. This is a fairly quick approach. I feel we want to improve UX the original issue soon, giving users feedback on what went wrong when they type something incorrect.

# Conflicts: # bin/near-cli.js

chadoh

I think the issue was .config(config) loaded src/config.js which has keys that weren't explicitly set as options.

What does this mean? I don't see any change related to .config(config)

I see eight commits here for a pretty small set of changes. I'd prefer we kept our commit history clean, so that it can be a good reference for us in the future. I don't think it makes sense to have more than one commit in this PR – do you mind squashing them together, either now or when you merge, and making sure the commit has a good description?

See a couple other comments & questions below. I don't currently feel informed enough to mark this as "approved" or as "request changes"

bin/near-cli.js

chadoh · 2020-03-31T19:01:43Z

bin/near-cli.js

+    .option('masterAccount', {
+        desc: 'Master account used when create new accounts',
+        type: 'string',
+        hidden: true


Why hide all of these?

This is a good debate to be had. The short answer is, there's a long road to getting near-shell consistent, and this is something of a bandaid. Actually, this whole PR is a bandaid.
Note that when you run: near to display the default help from yargs, it spits out commands and then the Options: section. These are flags that typically (again, we're working on it) apply to the top-level commands. Actually, the only good example of this is --accountId here.
Then, when you run near create_account --help you'll see the relevant Options: for that particular command, including --masterAccount.
This hidden key is not hiding it everywhere, but hiding it from the top-level list. I'm not against removing the hidden part, but it'll just make th

whoops. It'll just make that a bit longer. The fundamental issue here is we want to use strict mode, but we can't do that when we load config.js and it contains additional flags that aren't captured at the top level. So in this PR we're like, "yeah, you can give me a masterAccount flag here, it won't do much on its own, but you can"
And from there yargs's strict mode is possible whereby it'll scream at you if you misspell something or provide suggestions (recommendCommands) or tell you what you're missing.

Excellent explanation, thank you

test/test_account_operations.sh

test/test_generate_key.sh

janedegtiareva

Looks good! Have you tested it on windows? Do you mind commenting on the PR with how you tested it? LGTM after making sure it works on diff platforms

mikedotexe · 2020-03-31T21:48:50Z

Looks good! Have you tested it on windows? Do you mind commenting on the PR with how you tested it? LGTM after making sure it works on diff platforms

On Windows I run near fake and nothing happens. Then when I'm on this branch I can run:
node bin\near fake and it'll suggest a similar, valid command.

Same thing on OS X, except this time I'll use an errant argument.
near state --fake argie mikedotdmg
makes it look like everything is fine, leading the user to believe they may need this --fake argument.
running on this PR:
./bin/near state --fake argie mikedotdmg
It tells the user that fake isn't a thing.

The first line of this PR's description links to the issue created by Illia who was trying to run:
near transfer acc1 my_validator 1

This has the same behavior.

add yarg’s strict() to chain. add walletUrl, contractName as options

219aa28

mikedotexe requested a review from vgrichina February 6, 2020 19:20

vgrichina reviewed Feb 6, 2020

View reviewed changes

bin/near-cli.js Outdated Show resolved Hide resolved

add hidden key to walletUrl and contractName

6d4dece

mikedotexe added 6 commits March 31, 2020 08:35

Merge branch 'master' into add-config-vars-as-args

efc12ac

# Conflicts: # bin/near-cli.js

add masterAccount option in near-cli

7d35f56

fix test using delete_account to delete

3189a4e

fix test using defult to ci networkId

ae31dc2

fix lint spacing

6e2d01f

remove node 10 from travis

bd58809

mikedotexe mentioned this pull request Mar 31, 2020

Add Yargs default and middleware logic to display helpful message during invalid arg/command calls #262

Closed

chadoh reviewed Mar 31, 2020

View reviewed changes

mikedotexe requested a review from janedegtiareva March 31, 2020 20:30

mikedotexe added 2 commits March 31, 2020 13:56

fix typo for masterAccount description

dcf0d8a

in test_generate_key, use $NODE_ENV as flag and directory

bc7a73f

janedegtiareva approved these changes Mar 31, 2020

View reviewed changes

chadoh approved these changes Mar 31, 2020

View reviewed changes

mikedotexe merged commit 2932fb2 into master Mar 31, 2020

chadoh deleted the add-config-vars-as-args branch April 1, 2020 00:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add yarg’s strict() to chain. add walletUrl, contractName as options #265

add yarg’s strict() to chain. add walletUrl, contractName as options #265

mikedotexe commented Feb 6, 2020 •

edited

Loading

vgrichina commented Feb 6, 2020

vgrichina commented Feb 6, 2020

mikedotexe commented Feb 6, 2020

mikedotexe commented Feb 6, 2020

chadoh left a comment

chadoh Mar 31, 2020

mikedotexe Mar 31, 2020

mikedotexe Mar 31, 2020

chadoh Mar 31, 2020

janedegtiareva left a comment

mikedotexe commented Mar 31, 2020

add yarg’s strict() to chain. add walletUrl, contractName as options #265

add yarg’s strict() to chain. add walletUrl, contractName as options #265

Conversation

mikedotexe commented Feb 6, 2020 • edited Loading

vgrichina commented Feb 6, 2020

vgrichina commented Feb 6, 2020

mikedotexe commented Feb 6, 2020

mikedotexe commented Feb 6, 2020

chadoh left a comment

Choose a reason for hiding this comment

chadoh Mar 31, 2020

Choose a reason for hiding this comment

mikedotexe Mar 31, 2020

Choose a reason for hiding this comment

mikedotexe Mar 31, 2020

Choose a reason for hiding this comment

chadoh Mar 31, 2020

Choose a reason for hiding this comment

janedegtiareva left a comment

Choose a reason for hiding this comment

mikedotexe commented Mar 31, 2020

mikedotexe commented Feb 6, 2020 •

edited

Loading